Skip to content

Replace all places that can call Dir.chdir in thread with passing chdir to Open3.popen2e#60

Merged
gyfelton merged 8 commits intomasterfrom
gyfelton/fix-chdir-warning
Sep 1, 2023
Merged

Replace all places that can call Dir.chdir in thread with passing chdir to Open3.popen2e#60
gyfelton merged 8 commits intomasterfrom
gyfelton/fix-chdir-warning

Conversation

@gyfelton
Copy link
Member

@gyfelton gyfelton commented Aug 31, 2023

This elimiates the warning: "conflicting chdir during another chdir block" introduced in 1.4.0 so it can now run smoothly in Ruby 3 instead of throwing exception because of https://bugs.ruby-lang.org/issues/15661

Tested with a repro case on our side that is no longer failing with ruby 3.
And submodules are cloned fine since git submodule status shows correct refs for each and the repo size is correct. (If not done correctly, checkout will happen too quick and final repo size will be too small)

While cd will change where the current dir is, because it happens in a thread it should not impact the subsequent operations. This is also what used be before #53 is landed
@gyfelton gyfelton requested review from ndizazzo and timsutton August 31, 2023 20:55
fail_on_error(*cmd, quiet: !verbose, print_on_failure: print_git_errors)
end
fail_on_error(['cd', File.join(abs_clone_path, pwd).to_s], quiet: !verbose)
cmd = ['cd', File.join(abs_clone_path, pwd).to_s, '&&', 'git', 'submodule',
Copy link
Collaborator

@justinseanmartin justinseanmartin Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple things here:

  1. I don't think that running cd in a subshell actually changes the current working directory of the container ruby process.
2.7.4 :003 > BuildExecution.fail_on_error('cd', '/var/tmp')
D, [2023-08-31T17:20:32.130055 #35234] DEBUG -- build_execution: Running Shell Safe Command: "cd" "/var/tmp"

 => "" 
2.7.4 :004 > BuildExecution.fail_on_error('pwd')
D, [2023-08-31T17:20:34.590263 #35234] DEBUG -- build_execution: Running Shell Safe Command: "pwd"

/Users/jmartin/Development/ios-register-worktree
  1. fail_on_error doesn't know how to process && (that requires bash interpreter to interpret that boolean). I'm pretty sure this will pass the && as a string literal parameter to the cd command handler. I don't know why cd isn't erroring out in this case, but I don't think it will do what you think it should be doing. Example:
2.7.4 :002 > BuildExecution.fail_on_error('echo', 'hi', '&&', 'echo', 'bye')
D, [2023-08-31T17:25:55.820719 #36253] DEBUG -- build_execution: Running Shell Safe Command: "echo" "hi" "&&" "echo" "bye"

hi && echo bye
2.7.4 :003 > BuildExecution.fail_on_error('cd', '/var/tmp', '&&', 'echo', 'bye')
D, [2023-08-31T17:26:11.979083 #36253] DEBUG -- build_execution: Running Shell Safe Command: "cd" "/var/tmp" "&&" "echo" "bye"

 => "" 
 2.7.4 :007 > BuildExecution.fail_on_error('cd', '/users', '&&', 'pwd')
D, [2023-08-31T17:36:44.296645 #36695] DEBUG -- build_execution: Running Shell Safe Command: "cd" "/users" "&&" "pwd"

 => "" 

In the second and third commands above, I'd expect it to return "bye\n" and the current working directory if it actually worked as expected.

I think what you want to do is pass , chdir: <path> in the fail_on_error opts arg, which will get plumbed through to Open3.popen2e.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha! Thx I should have tested out above myself!

i am also thinking if doing threading here might not help improve anything at all? because git operations might still be done in serial anyway.
Testing all out now since i know the expected state of our case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about git -C? You can prepend any git command with that from what I can tell. It works like tar -C

image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see the “What is the way set pwd when using multithreaded ruby to write build shell scripts?” discussion is alive and well.

I recall that when fail_on_error and friends were just a prototype, the chdir option had to be plumbed in precisely because:

  1. calling chdir() all over the place had dangerous threading/global state implications.
  2. Not every command was nice like git and gave you an option to swap out the pwd on the command line.
  3. fail_on_error was designed to avoid ever invoking the shell to cut down on unintended consequences from things like shell tokenization, and to keep the performance overhead of starting the shell to zero….

… But people mostly use the shell to change pwd…so it’s very natural to want something

Anyway, The chdir keyword argument is there to be used, and this discussion is almost old enough to be memorizing its times tables in math class 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will do both: this version (1.4.3) keeps the current chdir implementation since at high level it's the "same" as current impl just doing in the right way
And in a minor version bump (1.5.0) change it to -C since it now depends on git side implementation which might have other implications.

@gyfelton gyfelton changed the title Replace two places that can call chdir in thread with cd dir Replace two places that can call Dir.chdir in thread with passing chdir to Open3.popen2e Sep 1, 2023
@gyfelton gyfelton changed the title Replace two places that can call Dir.chdir in thread with passing chdir to Open3.popen2e Replace all places that can call Dir.chdir in thread with passing chdir to Open3.popen2e Sep 1, 2023
@gyfelton gyfelton merged commit d05d049 into master Sep 1, 2023
@gyfelton gyfelton deleted the gyfelton/fix-chdir-warning branch September 1, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants